-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add warning for binary string in "application/json" or parameter #139
Conversation
aa5da53
to
99b7580
Compare
Codecov Report
@@ Coverage Diff @@
## master #139 +/- ##
==========================================
+ Coverage 92.99% 93.22% +0.22%
==========================================
Files 59 60 +1
Lines 2156 2229 +73
==========================================
+ Hits 2005 2078 +73
Misses 151 151
Continue to review full report at Codecov.
|
99b7580
to
6db9bae
Compare
6db9bae
to
a75e405
Compare
a75e405
to
c3841cb
Compare
c3841cb
to
37d6184
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is heading in the right direction but I left some comments with suggested improvements or questions.
37d6184
to
f79022b
Compare
f79022b
to
2fd0c5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few minor changes to make and then potentially larger changes in how we handle the configuration situation. I also would like to see one positive test that proves the correct usage of a binary property will not create a warning.
1802907
to
5823533
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good. Probably the last thing to do is discuss the config stuff. I'll think about it and get back to you
0d6fe7f
to
b72f2cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One little thing to address, but looks good otherwise! I'm approving but I'll wait to merge in case @mkistler wants to take one more look at it
b72f2cb
to
b36b036
Compare
b36b036
to
90aec33
Compare
…r parameter - added a findOctetSequencePaths function to handle the logic of finding schema type: string, format: binary for cases of nested arrays, objects, nested arrays of type object, objects with properties that are nested arrays, and objects with properties that are objects, and the simple case where a schema uses type: string, format: binary directly. This function takes a schema object from a resolvedSpec and returns a list of paths to octet sequences (empty list if none found). The function accepts the path both as an array and a string and returns the path in the same format received. - added logic to handle application/json request bodies that use schema type: string, format: binary - added logic to handle application/json response bodies of type: string, format: binary - added logic to handle parameters of type: string, format: binary - removed 'binary' as a valid format for type: string parameters. parameters of type: string, format: binary will result in "type+format not well-defined" error - added tests to ensure warnings are issued for request bodies, response bodies, and parameters with schema, type: string, format: binary - added complex tests to exercise combinations of nested arrays and objects that contain schema type: string, format: binary (complex tests done on response bodies) - added "json_or_param_binary_string" as to .defaultsForValidator as a warning in the oas3.schemas section - added "json_or_param_binary_string" configuration option to the README.md rules and defaults sections in the oas3 schemas section - changed findOctetSequencePaths to accept the path both as a string and an array.
90aec33
to
81eacff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'll see if @mkistler wants to take one more look and then merge this morning
Requested changes have been addressed
# [0.21.0](v0.20.0...v0.21.0) (2020-02-18) ### Features * add validator warning for binary string in "application/json" or parameter ([#139](#139)) ([9497333](9497333))
🎉 This PR is included in version 0.21.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Changes:
Refactoring in responses.js:
Tests: